Emit affine maps from AutoTP layers - #8519
Achyuthan-S wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 129087a461
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 'original_shape': _normalize_uc_shape(original_shape), | ||
| 'is_bias': is_bias, | ||
| 'replicated': replicated, | ||
| 'affine_map': affine_map, |
There was a problem hiding this comment.
Keep the conversion metadata test in sync
Adding affine_map unconditionally changes every _build_param_uc_restore_meta() conversion dictionary, but tests/unit/runtime/tensor_parallel/test_autotp_universal_checkpoint.py:235-244 still compares that dictionary exactly without this key, so the existing unit test now fails even when the argument uses its default. Update that test—preferably to assert only the stable fields rather than the entire private dictionary—or omit the key when no map is supplied.
AGENTS.md reference: AGENTS.md:L30-L32
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Schema compatibility and embedding-path coverage issues remain, and the producer test does not enforce exact output.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds affine-map emission for AutoTP checkpoint metadata and uses producer-generated maps during conversion while preserving legacy categories.
Changes:
- Derives maps for supported contiguous and replicated layouts.
- Updates converter compatibility handling.
- Expands producer and resume test coverage.
File summaries
| File | Summary | Final review comments |
|---|---|---|
tests/unit/checkpoint/test_autotp_uc_checkpoint.py |
Tests producer-generated layouts and resume behavior. | Nit (2 votes): Assert the emitted pattern set exactly matches expectations. |
deepspeed/module_inject/layers.py |
Produces and collects affine maps. | Moderate (2 votes): affine_map=None changes the metadata schema and breaks an existing test. Moderate (1 vote): The embedding path does not provide affine maps for contiguous row-sharded layouts. |
deepspeed/checkpoint/ds_to_universal.py |
Consumes affine maps alongside legacy patterns. | No final comments. |
Review details
Suppressed comments (1)
deepspeed/module_inject/layers.py:713
- This collector only serializes
conversion_meta['affine_map'], but the AutoTP embedding path still constructs metadata directly inauto_tp.py:_slice_embeddingwithout supplying an affine map. Consequently a contiguous row-sharded embedding continues through the category fallback and this producer does not cover all contiguous AutoTP layouts described by the PR; derive and pass the map for that path as well.
affine_map = conversion_meta.get('affine_map')
if affine_map is not None:
affine_maps[pattern] = affine_map.to_dict()
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 'original_shape': _normalize_uc_shape(original_shape), | ||
| 'is_bias': is_bias, | ||
| 'replicated': replicated, | ||
| 'affine_map': affine_map, |
| for pattern, want in expected.items(): | ||
| assert pattern in maps, f"producer emitted no affine map for {pattern}" | ||
| assert maps[pattern] == want, (f"emitted map for {pattern} differs from the layout the resume " | ||
| f"fixture verifies:\n emitted {maps[pattern]}\n expected {want}") |
7041ce0 to
fd3dec7
Compare
|
Both fixed in 7041ce0. I've taken the second option: affine_map is omitted when there's no map, so the dict an existing layer publishes is unchanged and test_param_uc_restore_builder_normalizes_shapes_and_nests_conversion_view passes without modification. It's also stored as to_dict() now rather than a live object, keeping the conversion schema plain scalars like every other field. On the producer test — also right. The docstring claimed "exactly the four maps" but the loop only checked presence, so a stray extra map would have passed. It now asserts the emitted pattern set equals the expected one first. cpu-torch-latest is green on the full suite, which covers the test above. |
|
Is there test that checks whether all legacy mapping that could be convert to affine had been converted? I was told embedding is not converted. |
|
Added that test — It turned up the embedding case. Map derivation hangs off The test fails without that change, naming 75 tests pass locally: the affine suite, the producer test, this one, the four resume cases, and |
187c563 to
e385ba2
Compare
|
Added TestUnevenVocabCrossTpResume in 44f5ba1: a 101-row vocabulary head saved at TP2, converted, and restored at TP1 and TP2. The 51/50 split is the point — TP2 → TP1 merges two unequal shards, which is where an even-split assumption would show up. The fixture asserts the emitted map records that split before saving, so the test fails if conversion falls back to the vocabulary category rather than passing quietly. Adapted from @jinyouzhi's cross-tp vocab test in #8309, using the column partition already in tree so it carries no dependency on that PR. |
Derive a parameter's affine map where the layer already knows its per-rank extents. The conversion metadata does not carry them -- they are resolved by _freeze_partition_sizes while the layer is built and are not recoverable from a shape alone -- so the map is built at mark time rather than at collection. Publish the maps alongside the existing pattern lists, so a converter that predates them still reads the checkpoint through the categories. The converter that prefers a map therefore has to mark those patterns as superseded, or a strict conversion rejects them as unused. The resume fixture now takes its layout from the producer instead of supplying one, so the four save-convert-resume cases exercise the metadata a real job writes. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
Adding the key unconditionally changed the conversion schema for every parameter, including layouts with no map to describe. Emit it only where there is one, so an existing layer publishes exactly what it published before, and store it as plain scalars like the rest of the schema. Require the producer test to match the expected pattern set exactly, so an unintended extra map is a failure rather than something the test ignores. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
An untouched parameter is identical on every rank, so it is describable as one replicated piece -- but building the map needs the tp degree, which only the partitioned layers carry. Take it from one of those and fill in the rest after the walk. Add the coverage invariant: every parameter placed by a name category must also carry a map, or conversion still depends on the category. Only the layouts AutoTP refuses to describe are exempt. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
The producer findings belong in the contract rather than only in review: a map is derived where a layer records its metadata, because collection cannot see per-rank extents, and a parameter the machinery never touches still needs one or it falls back to its name category. State as a rule that a reader preferring the map must mark the category patterns superseded, since a strict conversion otherwise rejects them. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
101 rows over two ranks gives shards of 51 and 50, so the map has to carry the per-rank extents rather than assume an even split, and restoring at TP1 merges two unequal shards into one tensor. The fixture asserts the emitted map records that split before saving, so the test fails if conversion silently falls back to the vocabulary category. Adapted from @jinyouzhi's cross-tp vocab test in deepspeedai#8309, using the column partition already in tree so it carries no dependency on that PR. Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
44f5ba1 to
2b78c9c
Compare
Step 4 of the staging plan in #8252: AutoTP layers now emit an affine map, and the converter reads it on a path a real job takes rather than one a test supplies.
Follows #8385 (the IR, the lowering constructors, the converter branch) and #8477 (the scale fix). Scope here is contiguous splits and replicated parameters; the fused QKV and Yuan shared-QK layouts are next and still carry
unsupported_reason, so they emit no map and are unaffected.Where a map is built, and why it is not where you would expect
The producer cannot build a map from what
collect_autotp_universal_checkpoint_infosees. The conversion metadata carries eight fields and per-rank partition sizes are not among them — those were deliberately restore-only. But the layer does have them:_freeze_partition_sizesresolves them viaget_shard_size_listwhile the layer is built, and they are not recoverable later from a shape alone.So the map is derived at mark time, in
_set_param_uc_meta, which already receivespartition_sizes,sub_param_shard_widths,logical_shapeandreplicated, and hastp_world_sizein scope. That puts the derivation in one place rather than in each of the seven_mark_uc_metadataimplementations, andcollect_...then gathers what the layers produced.A layout that is not describable yet returns
None, so conversion falls back to the categories.The map and the categories have to coexist
A checkpoint carrying an affine map also carries the existing pattern lists, so a converter predating the map still reads it — that is the additivity §6.3 promises.
The consequence is that the converter which prefers the map never consults those branches, which leaves their patterns looking unused and fails a strict conversion. They are superseded, not unused, so the affine branch now marks them consumed. This is the compatibility question raised as "one boundary for review" in #8385; it turns out both have to be present and the reader has to account for the other.
This only surfaced end to end. The producer's output was correct in isolation, and the emitted maps matched a layout already verified by a passing resume — the failure was a strict-mode assert on rank 0 during conversion, which presented as rank 1 blocking on the following barrier.
Tests
TestAffineMapProducerbuilds a real AutoTP engine at TP2 and requires the producer to emit exactly the four maps the resume fixture previously supplied by hand. Those maps are not a guess: a full train → save → convert → resume cycle reproduces uninterrupted training through them, so matching them is evidence rather than self-consistency.affine_resume_checkpointnow takes its layout from the producer instead of injecting one, so the four save-convert-resume cases exercise the metadata a real job writes. All four pass — TP2 → TP1 and TP2 → TP2, through both the legacy and affine paths, compared against uninterrupted training on logits, losses, gradients, FP32 weights, both Adam moments and step counters.Validated on CPU/gloo (DS_ACCELERATOR=cpu LOCAL_SIZE=4): 109 passed across the resume matrix, the producer and coverage tests, the vocab cross-TP test, the affine unit suite and tests/unit/runtime/tensor_parallel/. The five remaining failures in that file are FusedAdam JIT-compile errors on this machine and are identical on upstream master.
Review findings
Three things came out of review, all fixed here:
- affine_map was added to every conversion metadata dict, changing the schema even for layouts with no map. Now emitted only where there is one, so an existing layer publishes exactly what it did before.
- The producer test checked that the expected maps were present but not that nothing extra was. It now asserts the pattern set exactly.
- Parameters the TP machinery leaves untouched — embeddings, norm weights — were classified TP_REPLICATED with no map, so conversion still fell back to the category. TestAffineMapCoverage now asserts the general invariant: every pattern in any category must carry a map, with only AutoTP-unsupported layouts exempt. It fails without the fix.
- TestUnevenVocabCrossTpResume covers a 101-row vocabulary head saved at TP2 and restored at TP1 and TP2. The 51/50 split is the point — restoring at TP1 merges two unequal shards, where an even-split assumption would surface. The fixture asserts the emitted map records that split before saving. Adapted from @jinyouzhi's cross-tp test in #8309, using the column partition already in tree so it carries no dependency on that PR.
Related: #8252, #8230.
cc @delock